Skip to content

refactor(wren): unify datasource resolution from connection_info#1506

Merged
douenergy merged 2 commits intoCanner:mainfrom
goldmedal:chore/cli-skill-enhance
Apr 2, 2026
Merged

refactor(wren): unify datasource resolution from connection_info#1506
douenergy merged 2 commits intoCanner:mainfrom
goldmedal:chore/cli-skill-enhance

Conversation

@goldmedal
Copy link
Copy Markdown
Contributor

@goldmedal goldmedal commented Apr 2, 2026

Summary

  • Remove --datasource flag from query, dry-run, validate CLI commands — datasource is now always read from connection_info.json
  • Keep --datasource / -d only on dry-plan for transpile-only use without a connection file
  • Add _normalize_conn to auto-flatten the MCP/web envelope format ({"datasource": ..., "properties": {...}}) so both flat and nested connection files work
  • Update docs (cli.md, connections.md) and CLI skill (SKILL.md, wren-sql.md)

Test plan

  • wren --sql 'select customer_id from customers' -l 3 works with envelope-format connection_info.json
  • wren dry-plan --sql '...' works with auto-discovered datasource from connection file
  • wren dry-plan --sql '...' -d duckdb works with explicit datasource override

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Documentation

    • Added a detailed SQL-rewrite reference explaining CTE injection and authoring rules.
    • Updated CLI and connection docs to declare where datasource is read and accepted connection formats.
    • Expanded error-recovery guidance with a dry-plan diagnostic workflow and remediation tips.
  • New Features

    • Connection files accept both flat and MCP/web-envelope JSON shapes.
    • The --datasource/-d option is now supported only for dry-plan (allowing transpile-only runs without a connection file).

Remove --datasource flag from query/dry-run/validate commands — datasource
is now always read from connection_info.json. Only dry-plan retains
--datasource/-d for transpile-only use without a connection file.

Also add _normalize_conn to auto-flatten the MCP/web envelope format
({"datasource": ..., "properties": {...}}) so both flat and nested
connection files work seamlessly.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Apr 2, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 2, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fea795c2-a464-44be-9a02-bd4bfcd545d1

📥 Commits

Reviewing files that changed from the base of the PR and between 94e1e29 and 1f59246.

📒 Files selected for processing (2)
  • .claude/CLAUDE.md
  • wren/.claude/CLAUDE.md
✅ Files skipped from review due to trivial changes (2)
  • .claude/CLAUDE.md
  • wren/.claude/CLAUDE.md

📝 Walkthrough

Walkthrough

CLI datasource handling was refactored: the datasource value is read from connection_info.json (flat or MCP-envelope); --datasource was removed from most commands and retained only for dry-plan; a _normalize_conn() helper was added to auto-flatten envelope-shaped connection JSON; docs were expanded on SQL rewriting and connection formats.

Changes

Cohort / File(s) Summary
Skill Documentation
cli-skills/wren-usage/SKILL.md, cli-skills/wren-usage/references/wren-sql.md
Clarified that connection_info.json is authoritative for datasource; removed --datasource from general execution guidance; added new wren-sql.md describing CTE injection/rewrite pipeline, supported SQL patterns, authoring rules, and dry-plan-based error-diagnosis workflow.
CLI Documentation
wren/docs/cli.md, wren/docs/connections.md
Documented that datasource is sourced from connection_info.json or --connection-info; added accepted connection JSON formats (flat and MCP/web envelope) and auto-unwrapping; updated examples to show --datasource/-d only for dry-plan.
CLI Implementation
wren/src/wren/cli.py
Added _normalize_conn(conn: dict) to flatten envelope-style connection JSON; changed _resolve_datasource signature to accept conn_dict first; removed --datasource from main, query, dry-run, validate; reintroduced --datasource/-d only on dry-plan; adjusted engine construction to derive datasource from normalized connection dict.
Repository docs / meta
.claude/CLAUDE.md, wren/.claude/CLAUDE.md
Updated repository/module descriptions and CLI surface documentation to reflect wren/ module and dry-plan naming; added references to new query-rewriting components and memory subsystem in the agent context files.

Sequence Diagram(s)

(Skipped — changes are documentation and CLI parameter refactors without a new multi-component sequential runtime flow needing visualization.)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

python

Suggested reviewers

  • douenergy

Poem

🐰
I hop through JSON, flatten the nest,
Datasources settle where connections rest.
Dry-plan holds the lone little key,
CTEs hummed out for queries to see—
Docs and code dancing, tidy and blessed.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'refactor(wren): unify datasource resolution from connection_info' clearly and specifically describes the main change: centralizing how the datasource is resolved from connection_info.json and removing the --datasource flag from most commands.
Docstring Coverage ✅ Passed Docstring coverage is 90.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
wren/src/wren/cli.py (2)

287-309: dry-plan lacks --connection-info option unlike other commands.

The dry-plan command accepts --connection-file but not --connection-info (inline JSON). While this is likely intentional since dry-plan doesn't need full connection credentials, users who are accustomed to passing inline JSON for other commands may be confused.

Consider either adding --connection-info for consistency (ignoring all fields except datasource), or documenting this difference explicitly in the help text.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wren/src/wren/cli.py` around lines 287 - 309, The dry_plan CLI command
(function dry_plan) is missing the --connection-info / ConnInfoOpt option for
consistency with other commands; add an optional parameter matching the existing
ConnInfoOpt type (or reuse the same CLI annotation used elsewhere) to the
dry_plan signature alongside connection_file, accept inline JSON but only use
its datasource field when resolving datasource (i.e., pass the parsed
connection_info into _load_conn/_resolve_datasource or ignore credentials), and
update the help string to note that only datasource is considered so behavior
matches other commands.

50-60: Envelope flattening may discard extra top-level keys.

If the envelope format contains additional top-level keys besides datasource and properties (e.g., metadata fields added by MCP server), they will be silently dropped. If this is intentional, consider adding a brief comment. If not, you may want to merge them:

♻️ Optional: preserve additional top-level keys
 def _normalize_conn(conn: dict) -> dict:
     """Flatten the ``{"datasource": ..., "properties": {...}}`` envelope.

     MCP / web connection files wrap connection fields under a ``properties``
     key.  This normalises both formats into ``{"datasource": ..., **fields}``.
     """
     if "properties" in conn and isinstance(conn["properties"], dict):
         props = conn["properties"]
         props["datasource"] = conn.get("datasource", props.get("datasource"))
+        # Preserve any additional top-level keys (e.g., metadata)
+        for key, value in conn.items():
+            if key not in ("datasource", "properties") and key not in props:
+                props[key] = value
         return props
     return conn
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wren/src/wren/cli.py` around lines 50 - 60, The _normalize_conn function
currently returns only props when "properties" exists, dropping any other
top-level keys; update it to merge top-level keys with the flattened properties
instead of discarding them: create a new dict that starts from conn (excluding
the original "properties" key) and then update/override it with props, ensure
"datasource" is taken from conn if present else from props, and return this
merged result; modify the logic inside _normalize_conn to build and return that
merged dict while removing "properties".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@wren/src/wren/cli.py`:
- Around line 287-309: The dry_plan CLI command (function dry_plan) is missing
the --connection-info / ConnInfoOpt option for consistency with other commands;
add an optional parameter matching the existing ConnInfoOpt type (or reuse the
same CLI annotation used elsewhere) to the dry_plan signature alongside
connection_file, accept inline JSON but only use its datasource field when
resolving datasource (i.e., pass the parsed connection_info into
_load_conn/_resolve_datasource or ignore credentials), and update the help
string to note that only datasource is considered so behavior matches other
commands.
- Around line 50-60: The _normalize_conn function currently returns only props
when "properties" exists, dropping any other top-level keys; update it to merge
top-level keys with the flattened properties instead of discarding them: create
a new dict that starts from conn (excluding the original "properties" key) and
then update/override it with props, ensure "datasource" is taken from conn if
present else from props, and return this merged result; modify the logic inside
_normalize_conn to build and return that merged dict while removing
"properties".

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 75cbf458-5e3e-4cc4-9ee0-78d67de90e86

📥 Commits

Reviewing files that changed from the base of the PR and between f565a45 and 94e1e29.

📒 Files selected for processing (5)
  • cli-skills/wren-usage/SKILL.md
  • cli-skills/wren-usage/references/wren-sql.md
  • wren/docs/cli.md
  • wren/docs/connections.md
  • wren/src/wren/cli.py

Add wren/ as fifth main module in project-level docs, document
wren SDK build commands, cli-skills/, and PyPI publishing.
Update wren package docs with memory module, expanded connector
details, updated build commands, and test structure.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@goldmedal goldmedal requested a review from douenergy April 2, 2026 02:22
@douenergy douenergy merged commit 8f15e8c into Canner:main Apr 2, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants